Skip to content

Conversation

@naveensanjula975
Copy link
Member

No description provided.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements a password reset functionality using OTP (One-Time Password) instead of JWT tokens, adds email notification capabilities using Nodemailer, and makes minor adjustments to file upload validation. The changes also include database schema updates to store OTP data and updated documentation.

Changes:

  • Replaced JWT-based password reset with OTP-based system (6-digit codes with 15-minute expiry)
  • Added email service integration using Nodemailer for sending password reset codes
  • Added database fields (resetOTP, resetOTPExpiry) to User model with corresponding migrations
  • Removed GIF support from image upload middleware (now supports JPEG, PNG, WebP only)
  • Added debug logging to validation and upload middleware

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.

Show a summary per file
File Description
src/services/emailService.js New email service for sending password reset OTPs via SMTP
src/routes/auth.js Updated forgot-password and reset-password endpoints to use OTP instead of JWT
src/models/user.js Added resetOTP and resetOTPExpiry fields to User model
src/database/migrations/20251220165748-add-reset-otp-fields.js Empty migration file (duplicate/incomplete)
src/database/migrations/20251220000000-add-reset-otp-fields.cjs Migration to add OTP fields to Users table
src/middleware/validation.js Added debug logging for validation
src/middleware/upload.js Removed GIF support and added debug logging
package.json Added nodemailer dependency (v7.0.11)
package-lock.json Lock file update for nodemailer
README.md Added database setup and migration instructions
.env.example Added SMTP configuration variables

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

const createTransporter = () => {
// Check if SMTP settings are present
if (!process.env.SMTP_HOST || !process.env.SMTP_USER || !process.env.SMTP_PASS) {
console.warn('⚠️ SMTP settings not missing. Email sending functionality might fail.');
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The warning message contains a logical error. It says "SMTP settings not missing" when it should say "SMTP settings missing" or "SMTP settings not found". The current message states the opposite of what the code is checking.

Suggested change
console.warn('⚠️ SMTP settings not missing. Email sending functionality might fail.');
console.warn('⚠️ SMTP settings missing. Email sending functionality might fail.');

Copilot uses AI. Check for mistakes.
tls: {
// Do not fail on invalid certs (useful for dev with self-signed certs)
// In production, you might want to remove this
rejectUnauthorized: false
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting rejectUnauthorized to false disables TLS certificate validation, which creates a security vulnerability by allowing man-in-the-middle attacks. While the comment mentions this is useful for development, it should be controlled by an environment variable to ensure it's never accidentally deployed to production. Consider using process.env.NODE_ENV === 'development' ? false : true or a dedicated SMTP_REJECT_UNAUTHORIZED environment variable.

Copilot uses AI. Check for mistakes.
Comment on lines +373 to +388
// Verify OTP logic
// 1. Check if OTP matches
if (!user.resetOTP || user.resetOTP !== otp) {
return res.status(400).json({
success: false,
message: 'Invalid OTP code'
});
}

// 2. Check expiry
if (!user.resetOTPExpiry || new Date() > new Date(user.resetOTPExpiry)) {
return res.status(400).json({
success: false,
message: 'OTP code has expired'
});
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The password reset OTP verification lacks brute force protection. An attacker could attempt unlimited OTP guesses within the 15-minute window. With only 6 digits (1,000,000 possible combinations), this creates a security vulnerability. Consider implementing one or more protections: 1) Add a failed attempt counter with account lockout after a few failures, 2) Implement rate limiting on this endpoint, or 3) Add exponential backoff delays after failed attempts.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +124
import nodemailer from 'nodemailer';

/**
* Email Service for sending emails using Nodemailer
*/

// Create reusable transporter
const createTransporter = () => {
// Check if SMTP settings are present
if (!process.env.SMTP_HOST || !process.env.SMTP_USER || !process.env.SMTP_PASS) {
console.warn('⚠️ SMTP settings not missing. Email sending functionality might fail.');
}

return nodemailer.createTransport({
host: process.env.SMTP_HOST,
port: parseInt(process.env.SMTP_PORT || '587'),
secure: process.env.SMTP_SECURE === 'true', // true for 465, false for other ports
auth: {
user: process.env.SMTP_USER,
pass: process.env.SMTP_PASS,
},
tls: {
// Do not fail on invalid certs (useful for dev with self-signed certs)
// In production, you might want to remove this
rejectUnauthorized: false
}
});
};

/**
* Send password reset OTP email
* @param {string} to - Recipient email address
* @param {string} otp - 6-digit OTP
* @param {string} userName - User's name
* @returns {Promise<void>}
*/
export const sendPasswordResetEmail = async (to, otp, userName = 'User') => {
try {
const transporter = createTransporter();

// Email HTML template
const htmlContent = `
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Password Reset</title>
<style>
body { font-family: Arial, sans-serif; line-height: 1.6; color: #333; max-width: 600px; margin: 0 auto; padding: 20px; }
.container { background-color: #f9f9f9; border-radius: 10px; padding: 30px; border: 1px solid #e0e0e0; }
.header { text-align: center; margin-bottom: 30px; }
.logo { font-size: 24px; font-weight: bold; color: #4FC3F7; }
.content { background-color: white; padding: 25px; border-radius: 8px; }
.otp-box { background-color: #f5f5f5; font-size: 32px; font-weight: bold; letter-spacing: 5px; text-align: center; padding: 15px; border-radius: 8px; margin: 20px 0; border: 2px dashed #4FC3F7; color: #333; }
.footer { margin-top: 30px; text-align: center; font-size: 12px; color: #666; }
.warning { background-color: #fff3cd; border-left: 4px solid #ffc107; padding: 12px; margin: 20px 0; font-size: 14px; }
</style>
</head>
<body>
<div class="container">
<div class="header">
<div class="logo">🔧 FixPoint</div>
<h2>Password Reset Code</h2>
</div>

<div class="content">
<p>Hello <strong>${userName}</strong>,</p>

<p>We received a request to reset your password. Use the code below to complete the process:</p>

<div class="otp-box">
${otp}
</div>

<div class="warning">
<strong>⏰ Expires in 15 minutes.</strong><br>
If you didn't request this code, you can safely ignore this email.
</div>
</div>

<div class="footer">
<p>This is an automated email from FixPoint Maintenance Management System.</p>
<p>&copy; 2025 FixPoint. All rights reserved.</p>
</div>
</div>
</body>
</html>
`;

const textContent = `
Password Reset Code

Hello ${userName},

Your password reset code is: ${otp}

This code expires in 15 minutes.
If you didn't request this, please ignore this email.

FixPoint Maintenance Management System
`;

// Send email
const info = await transporter.sendMail({
from: `"${process.env.EMAIL_FROM_NAME || 'FixPoint Support'}" <${process.env.EMAIL_FROM || process.env.SMTP_USER}>`,
to: to,
subject: 'Your Password Reset Code - FixPoint',
text: textContent,
html: htmlContent,
});

console.log('✅ Password reset email sent successfully:', info.messageId);
return info;

} catch (error) {
console.error('❌ Error sending password reset email:', error);
throw new Error('Failed to send password reset email');
}
};

export default {
sendPasswordResetEmail
};
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new password reset OTP functionality lacks test coverage. Given that the codebase has comprehensive testing (as evidenced by test files for other services and controllers), this critical security feature should have tests covering: OTP generation and storage, email sending, OTP validation (both valid and invalid cases), expiry checking, and password update flow. This is especially important for security-critical authentication flows.

Copilot uses AI. Check for mistakes.

// Verify OTP logic
// 1. Check if OTP matches
if (!user.resetOTP || user.resetOTP !== otp) {
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OTP comparison uses a simple string equality check (user.resetOTP !== otp) which is vulnerable to timing attacks. An attacker could potentially use timing differences to determine correct OTP digits one at a time. Use crypto.timingSafeEqual() for constant-time string comparison to prevent timing attacks. Convert both strings to buffers before comparison.

Copilot uses AI. Check for mistakes.
Comment on lines +303 to +321
// Save OTP to database
await userService.updateUser(user.id, {
resetOTP: otp,
resetOTPExpiry: otpExpiry
});

// Send Email
try {
await emailService.sendPasswordResetEmail(user.email, otp, user.name);
} catch (emailError) {
console.error('Failed to send email:', emailError);
// In a real scenario, you might want to rollback the DB change or return an error,
// but for security "blind" responses, we might still return success or a generic error.
// Here we return 500 because if email fails, user can't reset.
return res.status(500).json({
success: false,
message: 'Failed to send verification email. Please try again.'
});
}
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When email sending fails, the OTP is already saved in the database but the error response prevents the user from knowing about it. This means the OTP is stored but unusable. Consider either: 1) Wrapping both database update and email sending in a try-catch to rollback the OTP on email failure, or 2) Clearing the OTP from the database when email sending fails. Currently, the database contains an OTP that was never delivered to the user.

Copilot uses AI. Check for mistakes.
Comment on lines +76 to +86
npm run db:migrate
```

2. **Seed Data**: Populate the database with demo users and branches.
```bash
npm run db:seed
```

**To reset the entire database (Undo all migrations, remigrate, and re-seed):**
```bash
npm run db:reset
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The README.md references npm scripts (db:migrate, db:seed, db:reset) that don't exist in package.json. These scripts need to be added to package.json, or the README documentation should be updated to use the correct commands (e.g., npx sequelize-cli db:migrate).

Suggested change
npm run db:migrate
```
2. **Seed Data**: Populate the database with demo users and branches.
```bash
npm run db:seed
```
**To reset the entire database (Undo all migrations, remigrate, and re-seed):**
```bash
npm run db:reset
npx sequelize-cli db:migrate
  1. Seed Data: Populate the database with demo users and branches.
    npx sequelize-cli db:seed

To reset the entire database (Undo all migrations, remigrate, and re-seed):

npx sequelize-cli db:migrate:undo:all && npx sequelize-cli db:migrate && npx sequelize-cli db:seed

Copilot uses AI. Check for mistakes.
{ expiresIn: '1h' }
);
// Generate 6-digit OTP
const otp = Math.floor(100000 + Math.random() * 900000).toString();
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using Math.random() for OTP generation is cryptographically insecure as Math.random() is a pseudo-random number generator not suitable for security-sensitive operations. An attacker could potentially predict OTP values. Use crypto.randomInt() instead for cryptographically secure random number generation. For example: crypto.randomInt(100000, 1000000).toString()

Copilot uses AI. Check for mistakes.
Comment on lines +3 to +22
/** @type {import('sequelize-cli').Migration} */
module.exports = {
async up (queryInterface, Sequelize) {
/**
* Add altering commands here.
*
* Example:
* await queryInterface.createTable('users', { id: Sequelize.INTEGER });
*/
},

async down (queryInterface, Sequelize) {
/**
* Add reverting commands here.
*
* Example:
* await queryInterface.dropTable('users');
*/
}
};
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration file is empty and contains only template comments with no actual migration logic. The up and down methods are empty stubs. This appears to be a duplicate of the .cjs migration file (20251220000000-add-reset-otp-fields.cjs) which contains the actual implementation. This empty migration file should be removed to avoid confusion and potential issues during migration execution.

Suggested change
/** @type {import('sequelize-cli').Migration} */
module.exports = {
async up (queryInterface, Sequelize) {
/**
* Add altering commands here.
*
* Example:
* await queryInterface.createTable('users', { id: Sequelize.INTEGER });
*/
},
async down (queryInterface, Sequelize) {
/**
* Add reverting commands here.
*
* Example:
* await queryInterface.dropTable('users');
*/
}
};
/**
* This file is intentionally left empty.
*
* The actual "add reset OTP fields" migration is implemented in:
* 20251220000000-add-reset-otp-fields.cjs
*
* This .js stub exists only to prevent confusion; it is not used as a
* Sequelize migration and exports no migration logic.
*/

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +8
await queryInterface.addColumn('Users', 'resetOTP', {
type: Sequelize.STRING(10),
allowNull: true,
comment: '6-digit OTP for password reset'
Copy link

Copilot AI Feb 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The resetOTP column is defined to store the raw 6-digit password reset code in plaintext, which means anyone who can read the database (including via a potential SQL injection elsewhere) can see valid reset codes and hijack accounts before they expire. Instead of persisting the raw OTP value, store only a hashed representation (similar to password hashing) and compare hashes during verification so that a database compromise does not expose usable reset codes.

Copilot uses AI. Check for mistakes.
@pabasara20 pabasara20 merged commit 6bd555d into dev Feb 2, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants